variables/options should be required if TVariables is not empty/default
#11241
Conversation
… is empty or default
🦋 Changeset detectedLatest commit: 08793f5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| ...[options]: HasRequiredVariables< | ||
| TVariables, | ||
| [ | ||
| optionsWithVariables: QueryHookOptionsWithVariables< | ||
| NoInfer<TData>, | ||
| NoInfer<TVariables> | ||
| >, | ||
| ], | ||
| [options?: QueryHookOptions<NoInfer<TData>, NoInfer<TVariables>>] | ||
| > |
There was a problem hiding this comment.
I have decided to use the "one signature, tuple behind conditional approach" here, as staying with only one public overload will make errors more readable and "localized".
This is how an error would look like if we had two overloads - the full call is marked red and the error is hard to read:
Instead, using this approach, the error looks like this - only variables are highlighted and the error is much more readable:
There was a problem hiding this comment.
Also something to highlight in these screenshots: the interface name QueryHooksOptionsWithVariables is a lot clearer than something calculated inline, like QueryHookOptions<...> & { variables: TVariables }
| useQuery(query, { | ||
| variables: { | ||
| // @ts-expect-error on unknown variable | ||
| foo: "bar", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Indenting these tests like this to highlight where exactly I am expecting the error to be propagated to the user.
| @@ -9,7 +9,7 @@ export function Query< | |||
| TVariables extends OperationVariables = OperationVariables, | |||
| >(props: QueryComponentOptions<TData, TVariables>) { | |||
| const { children, query, ...options } = props; | |||
| const result = useQuery(query, options); | |||
| const result = useQuery<unknown, OperationVariables>(query, options); | |||
There was a problem hiding this comment.
Where this causes problems is in generic implementations like our Query HOC:
Similar errors would always appear if we go through with the "options/variables sometimes required" approach here - there is no way of implementing that without that result.
Luckily, as you can see there's an easy fix - but this might cause some minor churn.
size-limit report 📦
|
phryneas
changed the title
variables/options should only be required if TVariables is empty or defaultvariables/options should be required if TVariables is not empty/default
jerelmiller
force-pushed
the
release-3.9
branch
2 times, most recently
from
c80ba36 to
3f7eecb
Compare
|
@phryneas is this one ready to go and just needing a review? |
This would address #10857
I'm opening this PR now to get feedback on it - if we agree that this is the way forward, it would need to be done for all other hooks as well - maybe we can find a nice type helper for it, although the implicitly created interface with a name helps for autocomplete readability.
I would target 3.9 with this - I don't expect this to negatively impact anyone, but it should probably be in a patch release.
Checklist: